Skip to content

Conversation

Tomasz-Smelcerz-SAP
Copy link
Member

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP commented Sep 15, 2025

Description

Note to reviewers: I was really trying to keep this PR as minimal as possible. There are many things to fix/improve/cleanup and many times I've seen that, but I decided not to, in order to introduce as few changes as possible.
Keep that in mind when suggesting improvements. Do we really want to make this PR even bigger? I would rather prefer to make a note in the sources and then create a follow-up PR to clean all the notes. Just remember that the work remaining in #2526 and our other efforts (like simplifying/getting rid of templatelookup.go) will probably make most of these review-fixes obsolete or unnecessary.
Up to you.

Changes proposed in this pull request:

  • Component Descriptors (CD) are now read from the OCM repository (which usually is some OCI registry)
  • To read a CD, one must provide an OCM Component Name and a version. In our current setup, the OCM Component Name is available only in a ModuleReleaseMeta for a given Module.
  • It means that with this PR the ModuleReleaseMeta becomes mandatory
  • To keep this PR minimal, I did not remove code/tests related to the previous "modes" of operation, where CD was fetched from a ModuleTemplate and ModuleReleaseMeta was, in some cases, optional. This code requires cleanup in a separate PR. Of course when the test is failing, I had to refactor it
  • An interesting case: What to do if user configures a Module for which there is no ModuleReleaseMeta? Currently, when user configures a module on a Kyma, for which there's no ModuleTemplate, the Kyma is put in Warning state. To keep it consistent, I use the same error->status resolution strategy. To be discussed.
  • Code coverage for some packages (templatelookup) decreased. This is because some test cases were disabled. This is (mostly) a legacy code that will disappear anyway. A new method of resolving ModuleTemplates along with ModuleReleaseMeta should be introduced, but this is not a part of this PR. I think it is OK to temporarily "lower the bar" - we're in the middle of a bigger refactoring here. Related issues that will introduce the necessary cleanpup/changes are listed are listed here
  • There is an open discussion that focuses on the little detail: The OCI registry URL. Previously it was taken from the static ComponentDescriptor data embedded in the ModuleTemplate. Now we have at least two different values we can use: An explicitly configured one and the one from ComponentDescriptor, after it is successfully fetched (we still need OCI Registry URL for subsequent operations, like raw-manifest fetching). Which URL to choose? Please take a look at the discussion. I decided to rely on explicitly provided value as I had no feedback that I should chose a different strategy here.

Related issue(s)
#2601

Copy link

Manifests created with 'make dry-run-control-plane' changed! Please make sure to check if changes are needed in related repositories like management-plane-charts, runtime-watcher, etc.

@Tomasz-Smelcerz-SAP
Copy link
Member Author

We are blocked here by: <internal-repo>/kyma/test-infra/issues/781 - we need all the ModuleReleaseMeta to have OCM version related attributes. In particular spec.mandatory.version

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch from f5357db to 6fca147 Compare September 24, 2025 05:38
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP marked this pull request as ready for review September 30, 2025 06:31
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP requested a review from a team as a code owner September 30, 2025 06:31
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch from e258230 to 9d9eb17 Compare September 30, 2025 07:21
@lindnerby lindnerby linked an issue Sep 30, 2025 that may be closed by this pull request
1 task
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch 3 times, most recently from 0be33b5 to 3be024d Compare September 30, 2025 13:29
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP changed the title Feat/read component descriptor from oci registry feat: Read component descriptor from oci registry Sep 30, 2025
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch 3 times, most recently from 071775d to 6aee460 Compare October 1, 2025 08:47
Copy link

github-actions bot commented Oct 1, 2025

⚠️ Pipeline-related file changes detected! Please review if related updates (e.g. manifest generation or workflow adjustments) are required.

@ruanxin ruanxin added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2025
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch 3 times, most recently from d3509af to 5ac7c15 Compare October 7, 2025 18:52
Copy link

github-actions bot commented Oct 7, 2025

Manifests created with 'make dry-run-control-plane' changed! Please make sure to check if changes are needed in related repositories like management-plane-charts, runtime-watcher, etc.

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch from 1a7339b to 4552c9a Compare October 7, 2025 20:12
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP force-pushed the feat/read-component-descriptor-from-oci-registry branch 3 times, most recently from 916d93f to e3b1e83 Compare October 8, 2025 23:49
// where a single descriptor is used for multiple test cases with slightly different module names.
// Defining this fake here has the advantage of using the same internal
// deserialization logic as the real service, which makes this fake a bit more "real".
type FakeService struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should exist under _test package specifically.

@@ -0,0 +1,123 @@
package componentdescriptor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to componentdescriptor_test

@@ -0,0 +1,74 @@
package componentdescriptor
Copy link
Contributor

@ruanxin ruanxin Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to componentdescriptor_test, and refactor those internal function you are testing as suggestion given here: https://github.com/kyma-project/lifecycle-manager/pull/2736/files#r2432968414

"github.com/stretchr/testify/require"
)

func TestUnTar(t *testing.T) {
Copy link
Contributor

@ruanxin ruanxin Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This internal function is worth extracting into a dedicated struct member function, and inject into componentdescriptor Service. Once extracted, you can make it public and test it through its public method instead of testing this internal function directly.

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would I do that? Nobody else (no package API user) wants to use that function and making it public increases package API surface for no reason.
I could extract this to a separate package instead, but then: What's the difference between that and just making the function public? I am still exposing a "private" thing via public API (and to make things worse I introduce a new package for that), although nobody asks for it.
That's a bad practice, package APIs should be minimal and hiding implementation details is exactly what packages are for.
BTW: Function doesn't have to be exported to be testable in Go. If in doubt, please see this official go docs
If the test file is in the same package, it may refer to unexported identifiers within the package, as in this example[...]
This is supported and promoted way of writing tests in Go.

Copy link
Contributor

@ruanxin ruanxin Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In short, that approach makes your code far more testable. Sorry to say this, but if you keep sticking to the “nobody asked for it” mindset, you’ll end up with a codebase full of tightly coupled logic that’s extremely difficult to test or maintain.

What I’m suggesting here is actually a very basic principle from testable design: introducing proper abstractions between your dependencies. For example, if you extract those UnTar functions into a dedicated struct, you can easily provide a fake implementation for UnTar and other file-related operations. That way, any upstream code (like extractFileGetComponentDescriptor) depending on them can be tested in isolation with simple unit tests.

Let's take your current unit test for GetComponentDescriptor at the moment in your PR, It's actually not a unit test, more of an integration test already. You need to compress some data with tar, that's not necessary for unit testing this function. Basically, after you abstract those tar-related operations, you don't need to bother about this detail while testing GetComponentDescriptor. Do you see the value now?

Regarding your point that “a function doesn’t have to be exported to be testable in Go” — I’ve challenged this view several times during our testing discussions. The fact that something is possible doesn’t mean it’s recommended. Testing private methods directly often leads to unmockable, tightly coupled code. As a result, your higher-level logic becomes dependent on concrete implementations instead of abstractions — which defeats the purpose of unit testing.

This isn’t just a personal opinion; it’s consistent with best practices discussed in well-known books like “Unit Testing Principles, Practices, and Patterns” by Vladimir Khorikov, and “Clean Code” by Robert C. Martin — all of which emphasize abstraction, isolation, and avoiding testing internal details directly.

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that something is possible doesn’t mean it’s recommended.

Please back that statement up with some arguments. In my view this is recommended because it is explicitly documented, with an example in the most official, idiomatic, definitive resource for the Go language: it's standard library documentation. The example I am referring to is the first code example in that document. If that's not a recommendation to you, then what is?
A book written on how to write Java programs? Well, Go's not Java if you haven't figured that out already.
Go authors are perfectly aware of Java's rambling mantra "you should not test private things" and they explicitly state that it is not how you write tests in Go. Here's the evidence

By the way, it's not only Go authors that realize the value to test the code directly, without any artificial obstacles like the one existing in Java, where you don't test private methods, because the compiler doesn't let you to.
Take a look at more modern language like Rust, or even younger - Zig. Or an older one, like Python. You will be surprised.
Do you really think that these guys are under-educated, and they should "fix" their languages with the help of “Unit Testing Principles, Practices, and Patterns” or “Clean Code”? That's ridiculous.
The very fact that such languages exist today: Go, Rust, Zig, is the proof that their authors consider Java's ideas about testing private code to be incorrect (to put it mildly).

Maybe if you really believe in what's written by these Ivory Tower Java Architects you should switch back to Java? I am programming in Go and I am not going to align to a flawed "advices" written for a flawed langague which, in my opinion (and not only mine...) Java is.

Testing private methods directly often leads to unmockable, tightly coupled code.

It's just your opinion, not backed up with any argument so far.
Whether the code is coupled or not has nothing to to with how you test it. You can have tightly coupled code with all the testing done via public interface. This happens in Java, where you can't test private methods anyway, right?
And you can have loosely coupled code with all unit tests defined inside the package (in Go).
These two things are orthogonal.

In short, that approach makes your code far more testable.

The code is testable. If there's a code in internal/service/componentdescriptor package that can't easily be tested, show it or stop spreading FUD.
The fact that you have strange issues with testing unexported functions is your problem. Probably you should read again the Go docs on the subject? As it is supported and promoted practice for testing in Go.

if you keep sticking to the “nobody asked for it” mindset, you’ll end up with a codebase full of tightly coupled logic that’s extremely difficult to test or maintain.

Any argument for that? Exactly which code in the internal/service/componentdescriptor package you consider extremely difficult to test?

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take your current unit test for GetComponentDescriptor at the moment in your PR, It's actually not a unit test, more of an integration test already. You need to compress some data with tar, that's not necessary for unit testing this function. Basically, after you abstract those tar-related operations, you don't need to bother about this detail while testing GetComponentDescriptor. Do you see the value now?

This is the only thing I can agree with. Maybe indeed the current logic for reading files from Layers is too complex and it's worth extracting to a separate tiny component. But it always comes with a price: You also increase complexity (as the system has now more "moving parts") and if you make that public you put an additional requirement on the consumers of your package: You force them to provide the additional dependency. It complicates the API of an otherwise simple package and is not always worth it.
However, I'll give it a try to see what can be done with this "untarring".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ruanxin I extracted all the abstractions into dedicated struct fields.
You can take a look.

Copy link
Contributor

@ruanxin ruanxin Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit testing principles are language-agnostic. The reason why testing private methods (white-box testing) is generally not recommended should be viewed from a broader perspective.

Take the concrete example of GetComponentDescriptor. You also agree that this function involves multiple internal steps to accomplish its complex task, right? If you focus only on implementation details without considering testability, the code naturally grows into such a state, for instance, “decompressing data” seems like an internal detail, so you write an untar function for it. And since Go allows testing of private methods, you might decide to test it directly. Done. And this pattern repeats for other internal logic as well.

However, the real challenge appears when you try to unit test the higher-level function that depends on those private helpers, in this case, GetComponentDescriptor. Go merely gives you the ability to test private methods in isolation. What you lose, however, is the ability to abstract and decouple your logic. The more you rely on testing private code, the more tightly coupled your functions become.

The most natural way to reduce this coupling is to group related logic into dedicated components (structs) and introduce interfaces as abstractions for their dependencies, so called Dependency Inversion Principle (DIP). Again, this is a language-agnostic concept, not Go-specific.

I want to emphasize again that the Go authors never recommend white-box testing in any official documentation — including the one you mentioned. The documentation merely states that Go supports testing unexported methods; it does not say it’s a best practice.

Claiming “this is recommended because it’s documented” is logically incorrect — documentation describes capabilities, not recommendations. For example, you can name variables and functions in any format you like, but we still enforce camelCase conventions via linters, because it’s aligned with a best naming practice we all agreed.

My whole point and in line with general unit testing best practices, introducing white-box tests usually causes more side effects than benefits.

That’s why we should use it very cautiously and the case you mentioned is exactly one where it does not fit.

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you lose, however, is the ability to abstract and decouple your logic. The more you rely on testing private code, the more tightly coupled your functions become.

I need to see that in code. Please provide a code snipped that shows that testing unexported function "couples" that function to any other functions.

Claiming “this is recommended because it’s documented” is logically incorrect — documentation describes capabilities, not recommendations. For example, you can name variables and functions in any format you like, but we still enforce camelCase conventions via linters, because it’s aligned with a best naming practice we all agreed.

Well, the convention you mentioned is exactly a recommendation taken from Go's documentation. So documentation provides recommendations, isn't it? Both effective go and standard library docs are part of the broad Go documentation.

The documentation merely states that Go supports testing unexported methods; it does not say it’s a best practice.

Along this line of reasoning, this documentation also doesn't say that testing exported method is a best practice. It's merely mentioned.
But wait a second. The documentation is not just mentioning internal testing. It provides examples. This is an example for: How to write tests is Go and it's an important example, because it's the very first example you see.
Of course authors clearly want people to write the code in the way provided in the examples - that's what examples are for!

You don't provide examples for things you consider bad practice, or you have justified doubts about!
Quite the opposite: You provide examples to promote the language and the power it provides. Apparently Go authors consider the ability of internal testing as an advantage. They're proud about it. That's why it's shown as an example.

So the authors clearly want people to write the test code inside the package being tested and, in some cases, outside it, without giving any strong preference towards one way or the other. You're free to choose!
How different that is from Java's mantra: "You shouldn't write tests for private methods"?

Now you have to consider that Go authors are definitely familiar with the "best practices" you mentioned - if they're so widespread. We can assume they heard about these, right?
Why then, they decided to openly promote (examples!) "internal testing"?
Well maybe, because they don't share the concerns you raised?
Why then they are testing unexported functions in the Go's standard library code? This code serves as an example - and recommendation for how to write clean Go code - to thousands of Go devs around the world.
Maybe Go authors don't think that the "best practices" you mentioned are so "best"?

We can test that hypothesis: I will point you to ten places in Go's standard library where unexported function is tested, and not marked as //TODO: fix this unexported test.
And you will open bug requests to Go project saying that it violates "best practices from Clean Code" (or any other book of your choice), and suggest to extract the abstraction to a public one and test it "properly".
How about that?

"introducing white-box tests usually causes more side effects than benefits."

This contradicts the Computer Science research results that clearly indicate that you usually need a mixture of "White-Box" and "Black-Box" tests in order to verify software correctness. White-Box tests are necessary - mainly for detecting implementation-level bugs (buffer overflows, for example), which Black-Box tests cannot detect by definiton as it is prohibited to address, or focus on, implementation details in Black-Box tests.

Copy link
Contributor

@ruanxin ruanxin Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"You don't provide examples for things you consider bad practice, or you have justified doubts about!"

To counter this point, I just want to quote some paragraph from book - The art of unit testing

8.2.1 Testing private or protected methods
Private or protected methods are usually private for a good reason in the developer’s
mind. Sometimes it’s to hide implementation details, so that the implementation can
change later without the end functionality changing. It could also be for security-
related or IP-related reasons (obfuscation, for example).
When you test a private method, you’re testing against a contract internal to the
system, which may well change. Internal contracts are dynamic, and they can change
when you refactor the system. When they change, your test could fail because some
internal work is being done differently, even though the overall functionality of the
system remains the same.
For testing purposes, the public contract (the overall functionality) is all that you
need to care about. Testing the functionality of private methods may lead to breaking
tests, even though the overall functionality is correct.

Think of it this way: no private method exists without a reason. Somewhere down
the line there’s a public method that ends up invoking this method, or invokes a pri-
vate method that ends up invoking the method you’re interested in. That means that any private method is usually part of a bigger unit of work, or a use case in the system,
that starts out with a public API and ends with one of the three end results: return
value, state change, or third-party call (or all three).
With this viewpoint, if you see a private method, find the public use case in the sys-
tem that will exercise it. If you test only the private method and it works, that doesn’t
mean that the rest of the system is using this private method correctly or handles the
results it provides correctly. You might have a system that works perfectly on the
inside, but all that nice inside stuff is used horribly wrong from the public APIs.
Sometimes if a private method is worth testing, it might be worth making it public,
static, or at least internal and defining a public contract against any code that uses it.
In some cases, the design may be cleaner if you put the method in a different class
altogether. We’ll look at these approaches in a moment.
Does this mean there should eventually be no private methods in the code base?
No. With TDD, you usually write tests against methods that are public, and those pub-
lic methods are later refactored into calling smaller, private methods. All the while,
the tests against the public methods continue to pass.

image


// Component uniquely identifies an OCM Component.
// See: https://ocm.software/docs/overview/important-terms/#component-identity
type Component struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComponentId would be even more fitting as the structs name to reflect what it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usade and function during the code even call this differently already:
OCMIdentity
Would be good to align this and settle for a name, to talk about one thing from the beginning.

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to ComponentId

as for usages, I don't quite get it. If it's a variable, it's called ocmi, in one place (a field in a struct) it's using the same name as target type: ComponentId
If I missed some place, please let me know.

typical usage example now:

ocmi, err := ocmidentity.NewComponentId(testComponentName, testComponentVersion)

package name could be shortened to just ocmid but I don't know if it's not too cryptic.
I tried ocm but then we're conflicting with the "real" ocm library in one file, so it's better to be avoided.

}

// MustNew is a convenience constructor that panics if name or version are not provided.
func MustNew(name, version string) *Component {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this is not needed. It's even dangerous because someone might use it in production code.
Looks like this is anyways only needed in tests so any convenience function can live in test_utils.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved, renamed to: MustNewComponentId

}, nil
}

func (c *Component) Name() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are only read methods (similar to getters) they can even have a value receiver instead of pointer to further indicate that:
func (c Component) Name() string {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to value receiver


type DescriptorKey string

func GenerateDescriptorKey(ocmi ocmidentity.Component) DescriptorKey {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not find a better or unused place to put this comment 😄
Once settled for a consistent name for ocmidentity.Component you should also adapt the the parameter names throughout the whole PR. So either: ocmId or componentId or ocmCompId or ocmIdentity, whatever the agreed name of the domain entity will be then.

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as written above, it's ocmi ocmidentity.ComponentId
I tried to fix all occurrences, but maybe something sneaked through my filters. Please report if so.
BTW: ocmi because it's shorter that componentId and it contains an important prefix indicating that it's ocm-related.

return nil, fmt.Errorf("%w for %s", ErrLayerNil, commonErrMsg(ocmi))
}

compDescLayerDigest := ocmArtifactConfig.ComponentDescriptorLayer.Digest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until here all that is done is needed to get the digest. You can wrap everything in a private method getDescriptorLayerDigest(*) to indicate that. Also allows for a comment on the func to describe details.

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

ReadAll(r io.Reader) ([]byte, error) // Reads all data from the reader
}

type defaultExtractFileIOHelper struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what is the reasoning behind these internal interface and immediate struct implementations?
Can you think about using dependency injection for these helper functionalities?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see it the untarIoHelper as well as the defaultExtractFileIOHelper can be injected dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored. Will be injected as agreed with @ruanxin

- name: Create and apply ModuleReleaseMeta from the template-operator repo
working-directory: template-operator
if: ${{ matrix.e2e-test == 'kyma-metrics' ||
matrix.e2e-test == 'non-blocking-deletion' ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick info on why this testcase is not important anymore? I don't know the details.

ocmi := ocmidentity.MustNew(tc.moduleName, tc.moduleVersion)
got := cache.GenerateDescriptorKey(*ocmi)
assert.Equal(t, tc.want, string(got))
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to align some things here:

  • DescriptorKey could also get its own file if the test gets it? Or incorporate this test as well in cache_test.go.
  • If there is only one testcase there is no need for a table driven structure. So either we come up with more testcases here or have just one plain test.

@@ -0,0 +1,6 @@
package oci

// compiled only when running tests.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why introduce this indirection and not create a separate pkg for the wrapper isolated with its tests.
And then inject the wrapper dependency for consumers as usual.

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why introduce another package "B" just to test package "A"?
Packages should have a purpose. In this case, package "A" - internal/repository/oci can "handle itself" very well, we can write all the tests we need without introducing new public abstractions.
I guess "do not multiply entities beyond necessity" is still the correct approach?

return m.pullLayerResult, nil
}

func wrapAsTar(t *testing.T, fileName string, data []byte) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the bad concesquence that you deeply coupled tar related operation into GetComponentDescriptor, a good unit test doesn't require such low level detail. The reason I mentioned here #2736 (comment)

}

// New is a constructor that ensures that both name and version are provided.
func New(name, version string) (*Component, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go have naming convesion to initilze struct, normally should be New[struct name], so here NewComponent is better. Actually I also second @lindnerby suggestions, rename it to ComponentId or maybe better name

// IsDescriptorCached checks if the descriptor is in the cache.
// It temporarily stops the underlying DescriptorService to ensure the cache is used
// instead of DescriptorService lookup.
func IsDescriptorCached(ocmi ocmidentity.Component) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only private used by the test, please don't make it public

return ts
}

func (fs *FakeService) Stop() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fakeservice should in the _test package, and those function Stop, Resume, Register ... can be private.


It("And Manifest CR for the Mandatory Module should be created with correct Owner Reference", func() {
Eventually(checkMandatoryManifestForKyma).
Eventually(checkMandatoryManifestForKyma, Timeout, Interval).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

descriptor, err := provider.NewCachedDescriptorProvider().GetDescriptor(&moduleTemplateFromFile)
ocmi, err := ocmidentity.New("kyma-project.io/module/template-operator", testCase.descriptorVersion)
require.NoError(t, err)
descriptor, err := provider.NewCachedDescriptorProvider(
Copy link
Contributor

@ruanxin ruanxin Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get the point, for each test case, you create a FakeService specifically, then what's the meaning for that stopped field, each FakeService not sharing the state.

And even you need that stopped state, you current implementation is thread unsafe for concurrent tests, you need a mutext for update it's state.

Copy link

Manifests created with 'make dry-run-control-plane' changed! Please make sure to check if changes are needed in related repositories like management-plane-charts, runtime-watcher, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Read component descriptor from oci registry

5 participants